fix: use current() instead of approved_up_to_transaction() in codebase #1089
Conversation
nboyse
force-pushed
the
fix/use-current-instead-of-approved_up_to_transaction
branch
from
894a04e
to
c56a89c
Compare
|
I'm just about to push the button on a review, Tash. Sorry there're so many repeat comments, but I did stop before the end of my diff, so there may be more to follow - or not if some of my (repeated) comments are ill-founded! |
common/tests/test_models.py
Outdated
| models.TestModel1.objects.current().get().pk | ||
| == unapproved_version.pk | ||
| ) | ||
| assert models.TestModel1.objects.current().get().pk == unapproved_version.pk |
There was a problem hiding this comment.
You don't actually need to reference pk when testing for model instance equality, Django defines a Model.__eq__() method in order to do the right thing when comparing instances. Not applicable here, but generally it can avoid problems if an instance is None.
There was a problem hiding this comment.
Yeah this is fair, will update alongside other changes, it wasn't my test initially :)
common/querysets.py
Outdated
| @@ -66,7 +66,7 @@ def not_current(self, asof_transaction=None) -> QuerySet: | |||
| :param transaction Transaction: The transaction to limit versions to. | |||
| :rtype QuerySet: | |||
| """ | |||
| current = self.approved_up_to_transaction(asof_transaction) | |||
| current = self.current() | |||
There was a problem hiding this comment.
There's no longer an opportunity for the optional asof_transaction param to override current and so this may not be correct behaviour. May not because the use of the optional asof_transaction param seems to run counter to the name (and intent?) of this function.
quotas/business_rules.py
Outdated
| @@ -92,7 +85,7 @@ class ON5(BusinessRule): | |||
| def validate(self, origin): | |||
| if ( | |||
| type(origin) | |||
| .objects.approved_up_to_transaction(origin.transaction) | |||
| .objects.current() | |||
There was a problem hiding this comment.
current equivalence issue?
quotas/business_rules.py
Outdated
| measures = measures_models.Measure.objects.approved_up_to_transaction( | ||
| order_number_origin.transaction, | ||
| ) | ||
| measures = measures_models.Measure.objects.current() |
There was a problem hiding this comment.
current equivalence issue?
quotas/business_rules.py
Outdated
| .objects.approved_up_to_transaction( | ||
| quota_definition.transaction, | ||
| ) | ||
| .objects.current() |
There was a problem hiding this comment.
current equivalence issue?
quotas/business_rules.py
Outdated
| if quota_definition.sub_quota_associations.approved_up_to_transaction( | ||
| self.transaction, | ||
| ).exists(): | ||
| if quota_definition.sub_quota_associations.current().exists(): |
There was a problem hiding this comment.
current equivalence issue?
quotas/business_rules.py
Outdated
| association.main_quota.sub_quota_associations.approved_up_to_transaction( | ||
| association.transaction, | ||
| ) | ||
| association.main_quota.sub_quota_associations.current() |
There was a problem hiding this comment.
current equivalence issue?
common/models/tracked_qs.py
Outdated
| @@ -47,7 +47,7 @@ def latest_approved(self) -> TrackedModelQuerySet: | |||
| update_type=UpdateType.DELETE, | |||
| ) | |||
|
|
|||
| def current(self) -> TrackedModelQuerySet: | |||
| def current(self, transaction=None) -> TrackedModelQuerySet: | |||
There was a problem hiding this comment.
Introducing a transaction parameter changes the meaning / intent of this function. I think retaining and using approved_up_to_transaction() is a clearer way of filtering tracked models to up to a specific transaction that is not the current one.
There was a problem hiding this comment.
... TrackedModelQuerySet.current() is a convenience and short-cut to calling TrackedModelQuerySet.approved_up_to_transaction() for specific situations.
* Add quota order number create view * Add validation * Update Quota factory to pass validation * Edit confirm page and link to quota origin form * Update validation, add tests * Use correct url format * Add create quota link to workbasket page, refactor * Fix test runner error * Add migrations to alter regex validator * Address PR comments * Amend business rules
Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.8.5 to 3.8.6. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](aio-libs/aiohttp@v3.8.5...v3.8.6) --- updated-dependencies: - dependency-name: aiohttp dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
https://github.com/uktrade/tamato into fix/use-current-instead-of-approved_up_to_transaction
https://github.com/uktrade/tamato into fix/use-current-instead-of-approved_up_to_transaction
TP-2000-396 - Use current() instead of approved_up_to_transaction() in codebase
Why
approved_up_to_transaction() is largely redundant and the functionality that it holds instead sits under the current function instead.
Instead of passing around transactions between views and forms before supplying approved_up_to_transaction, we should use the current queryset method instead. This ticket is to replace any instances of the former with the latter.
What
Essentially replacing (almost) every instance of approved_up_to_transaction() with current(). This also means in some cases, the extra arguments that are passed to functions are also being removed as part of this. Thorough regression testing will be carried out by myself and the product manager to ensure this does not break any features within TAP.